Feature/cda 99 catalog ts extents versioned #1808
Conversation
00eca9b to
541b051
Compare
541b051 to
2d95ffb
Compare
| TimeSeriesDao dao = getTimeSeriesDao(dsl); | ||
|
|
||
| String tsId = requiredParam(ctx, NAME); | ||
| String office = ctx.queryParam(OFFICE); |
There was a problem hiding this comment.
office id should be required
There was a problem hiding this comment.
Updated to required
| String cursor = ctx.queryParam(PAGE); | ||
| int pageSize = ctx.queryParamAsClass(PAGE_SIZE, Integer.class).getOrDefault(DEFAULT_PAGE_SIZE); | ||
|
|
||
| ZonedDateTime begin = beginStr != null ? Controllers.queryParamAsZdt(ctx, BEGIN, timezone) : null; |
There was a problem hiding this comment.
Use Instant to send to the Dao, the Dao's don't need to care about time zones
There was a problem hiding this comment.
Updated to Instant.
| @Override | ||
| public TimeSeriesVersions getTimeSeriesVersions(String cursor, int pageSize, String names, String office, | ||
| ZonedDateTime begin, ZonedDateTime end) { | ||
| Condition condition = AV_CWMS_TS_ID2.CWMS_TS_ID.eq(names); |
There was a problem hiding this comment.
AV_CWMS_TS_ID2 - I believe this is the view that supports aliases. That should be advertised in the OpenAPI.
There was a problem hiding this comment.
added comment to description
| @Override | ||
| public TimeSeriesVersions getTimeSeriesVersions(String cursor, int pageSize, String names, String office, | ||
| ZonedDateTime begin, ZonedDateTime end) { | ||
| Condition condition = AV_CWMS_TS_ID2.CWMS_TS_ID.eq(names); |
There was a problem hiding this comment.
This should ignore case, the corrected case will be returned to the client
|
|
||
|
|
||
| @Override | ||
| public TimeSeriesVersions getTimeSeriesVersions(String cursor, int pageSize, String names, String office, |
There was a problem hiding this comment.
| public TimeSeriesVersions getTimeSeriesVersions(String cursor, int pageSize, String names, String office, | |
| public TimeSeriesVersions getTimeSeriesVersions(String cursor, int pageSize, String name, String office, |
| + "current location in the response stream. This is an opaque " | ||
| + "value, and can be obtained from the 'next-page' value in the response."), | ||
| @OpenApiParam(name = PAGE_SIZE, type = Integer.class, description = "How many entries per page returned. " | ||
| + "For JSON/XML paging, this controls page size. " |
There was a problem hiding this comment.
You call out XML and CSV in these docs, but the only accepted content type is json
There was a problem hiding this comment.
good catch. Copied over from the timeseries controller and missed that.
| @OpenApiParam(name = END, description = "Specifies the " | ||
| + "end of the time window for data to be included in the response. If" | ||
| + " this field is not specified, any required time window ends at the" | ||
| + " current time. " |
There was a problem hiding this comment.
I don't see the logic to contain to current time
There was a problem hiding this comment.
Removed part of comment containing this to current time. Don't see why a user couldn't want to get everything and not provide a time window
| + "in the response."), | ||
| @OpenApiParam(name = BEGIN, description = "Specifies the " | ||
| + "start of the time window for data to be included in the response. " | ||
| + "If this field is not specified, any required time window begins 24" |
There was a problem hiding this comment.
I don't see the 24 hour substitution
There was a problem hiding this comment.
I updated the comment. Don't think this makes sense for versions
| Record lastRecord = results.get(results.size() - 1); | ||
| Timestamp lastVersionTime = lastRecord.get(AV_TS_EXTENTS_UTC.VERSION_TIME); | ||
| if (lastVersionTime != null) { | ||
| builder.withNextPage(CwmsDTOPaginated.encodeCursor(DateUtils.toZdt(lastVersionTime).format(DateTimeFormatter.ISO_INSTANT), pageSize, total)); |
There was a problem hiding this comment.
Why are you transforming the version to ZonedDateTime instead of just staying at Insant?
There was a problem hiding this comment.
Updated to just use Instant
| .assertThat() | ||
| .statusCode(is(HttpServletResponse.SC_OK)) | ||
| .body("versions.size()", is(1)) | ||
| .body("next-page", notNullValue()) |
There was a problem hiding this comment.
recommend also testing the total and to make sure the extents aren't accidentally identical in both pages
There was a problem hiding this comment.
Added tests of total and confirmation data isn't identical in both pages
6091eb9 to
6895c9a
Compare
…ted to use Instants. Added readTheDocs for timeseries/versions
6895c9a to
b140f18
Compare
Summary
Implements timeseries/versions endpoint
Related Issue
Closes #779
Validation
Unit and Integration testing
Checklist